Skip to content

Conversation

@dzbarsky
Copy link
Contributor

@dzbarsky dzbarsky commented Sep 30, 2025

For anyone using rust_library_group (or similar, like rules_rs) the current setup forces flattening in every rdep. We can just carry them as a list instead and iterate it directly in the consumers.

We are slightly less efficient now when nesting a library group in a library group (which is probably very rare?).

The prost rules do incur flattening now when include_transitive_deps is enabled (which it's not by default, and it seems like a bad practice to not declare your deps properly?). In exchange, I've improved the depset handling for constructing those transitive deps (and I think we could further improve it by not constructing the transitive depset when the toolchain isn't configured to use it...)

This is technically a breaking change for folks who have custom rules creating CrateGroupInfo, although the adjustment should be pretty trivial.

@dzbarsky dzbarsky force-pushed the variant_dep_infos branch 4 times, most recently from 6bc3671 to 59807cc Compare September 30, 2025 01:07
Copy link
Collaborator

@illicitonion illicitonion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what the goal of this one is? Improving the depset construction sounds good, but why is a list the correct data type here? It feels like this is a maybe-nested maybe-has-duplicates-that-need-deduplicating collection of transitive providers?

elif rust_common.crate_group_info in dep:
dep_variant_transitive_infos.append(dep[rust_common.crate_group_info].dep_variant_infos)
elif CrateGroupInfo in dep:
dep_variant_infos.extend(dep[CrateGroupInfo].dep_variant_infos)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could dedupe here if we think folks have nested rust_library_groups with duplicated deps

@dzbarsky
Copy link
Contributor Author

I'm not sure what the goal of this one is? Improving the depset construction sounds good, but why is a list the correct data type here? It feels like this is a maybe-nested maybe-has-duplicates-that-need-deduplicating collection of transitive providers?

The problem is that the transitive providers can never be used efficiently as-is, because all the consumers want depsets of their nested fields, not the providers themselves. For example, cargo_build_script flattens the dep_variant_infos to extract the contained dep_infos, and then flattens each dep_info's transitive_build_infos to get the build_info.out_dir. We would need a depset of build_info_out_dirs (and similar usages) propagated on the DepVariantInfo to truly make use of the depset efficiently (Basically what we wanted was struct-of-arrays instead of array-of-structs). Given how it's actually being used in practice, there's no point wrapping the list of variant infos into a depset since every consumer just calls .to_list() on it anyway.

You're right that it could technically be transitive/duplicated if rust_library_group is nested inside another rust_library_group, though this isn't a usage I have ever come across; and in that edge case we can just dedupe/flatten once at the rust_library_group target instead of in all the consumers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants